-
Notifications
You must be signed in to change notification settings - Fork 2.1k
OpenAI.HttpClient to Azure.HttpClient mapper for test instrumentation #47416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a translation layer to enable the use of the test-proxy tool with the Stainless-generated OpenAI HttpClient. The implementation bridges Azure's HTTP pipeline (including policies, diagnostics, and retry logic) with OpenAI SDK clients, allowing test recording and sanitization to work correctly.
Key changes:
- Introduced HTTP client adapters to convert between OpenAI and Azure HTTP abstractions
- Added policy decoration capability to inject Azure pipeline policies into OpenAI HTTP clients
- Implemented URL building logic to construct Azure-compatible URLs from OpenAI request metadata
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
PolicyDecoratingHttpClient.java |
Wraps HttpClient with Azure pipeline policies for test instrumentation |
HttpClientHelper.java |
Main translation layer mapping OpenAI HttpClient to Azure HttpClient |
OpenAiRequestUrlBuilder.java |
Utility to construct absolute URLs from OpenAI request components |
AzureHttpResponseAdapter.java |
Adapts Azure HttpResponse to OpenAI HttpResponse interface |
AgentsClientBuilder.java |
Integrates the HTTP client mapper into client build methods |
PolicyDecoratingHttpClientTests.java |
Unit tests for policy decoration functionality |
HttpClientHelperTests.java |
Unit tests for request/response translation |
...e-ai-agents/src/test/java/com/azure/ai/agents/implementation/http/HttpClientHelperTests.java
Show resolved
Hide resolved
...s/src/test/java/com/azure/ai/agents/implementation/http/PolicyDecoratingHttpClientTests.java
Outdated
Show resolved
Hide resolved
...agents/src/main/java/com/azure/ai/agents/implementation/http/PolicyDecoratingHttpClient.java
Outdated
Show resolved
Hide resolved
...i-agents/src/main/java/com/azure/ai/agents/implementation/http/AzureHttpResponseAdapter.java
Outdated
Show resolved
Hide resolved
...ai-agents/src/main/java/com/azure/ai/agents/implementation/http/OpenAiRequestUrlBuilder.java
Show resolved
Hide resolved
sdk/ai/azure-ai-agents/src/main/java/com/azure/ai/agents/AgentsClientBuilder.java
Outdated
Show resolved
Hide resolved
...ai-agents/src/main/java/com/azure/ai/agents/implementation/http/OpenAiRequestUrlBuilder.java
Outdated
Show resolved
Hide resolved
.../azure-ai-agents/src/main/java/com/azure/ai/agents/implementation/http/HttpClientHelper.java
Show resolved
Hide resolved
sdk/ai/azure-ai-agents/src/main/java/com/azure/ai/agents/AgentsClientBuilder.java
Outdated
Show resolved
Hide resolved
| */ | ||
| AzureHttpResponseAdapter(com.azure.core.http.HttpResponse azureResponse) { | ||
| this.azureResponse = azureResponse; | ||
| this.headers = toOpenAiHeaders(azureResponse.getHeaders()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not eagerly do the mapping here. Response headers are not usually accessed. So, we can do this mapping lazily when headers method is called for the first time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did the same for body(). Because it returns an InputStream the responsibility to close the stream is pushed to the caller.
| if (this.httpClient == null) { | ||
| return null; | ||
| } | ||
| List<HttpPipelinePolicy> orderedPolicies = getOrderedCustomPolicies(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user only configures custom policies but not the HTTP client the policies are ignored and the default HTTP client will be used. We should use the provided pipeline policies in all cases.
| HttpClient decoratedHttpClient = getOpenAIHttpClient(); | ||
| return new ConversationsClient(getOpenAIClientBuilder().build().withOptions(optionBuilder -> { | ||
| if (decoratedHttpClient != null) { | ||
| optionBuilder.httpClient(HttpClientHelper.httpClientMapper(decoratedHttpClient)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will switch from OpenAI's HTTP client to Azure's HTTP client when there is a custom policy. Should we always use the Azure HTTP client to avoid different behaviors/HTTP clients? It would be good to have a consistent behavior regardless of how the client builder was configured. Or, we should clearly document that setting custom policy will result in a different HTTP client being used.
.../azure-ai-agents/src/main/java/com/azure/ai/agents/implementation/http/HttpClientHelper.java
Outdated
Show resolved
Hide resolved
.../azure-ai-agents/src/main/java/com/azure/ai/agents/implementation/http/HttpClientHelper.java
Outdated
Show resolved
Hide resolved
| private static HttpHeaders toAzureHeaders(Headers sourceHeaders) { | ||
| HttpHeaders target = new HttpHeaders(); | ||
| sourceHeaders.names().forEach(name -> { | ||
| List<String> values = sourceHeaders.values(name); | ||
| HttpHeaderName headerName = HttpHeaderName.fromString(name); | ||
| if (values.isEmpty()) { | ||
| target.set(headerName, ""); | ||
| } else { | ||
| target.set(headerName, values); | ||
| } | ||
| }); | ||
| return target; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here - this results in looping through each header twice. Instead, updating the Headers class to return a Map<String, List<String>> can be useful.
| try { | ||
| bodyData = toBinaryData(requestBody); | ||
| } finally { | ||
| closeQuietly(requestBody); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the source request body didn't close cleanly, then we should bubble that exception up as that indicates some issue with the underlying data source.
| return failedFuture(runtimeException); | ||
| } | ||
|
|
||
| return this.azureHttpClient.send(azureRequest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an exception is thrown, we should map that as well to OpenAI exception types.
| static URL buildUrl(HttpRequest request) { | ||
| try { | ||
| URI baseUri = URI.create(request.baseUrl()); | ||
| URL baseUrl = baseUri.toURL(); | ||
| String path = buildPath(baseUrl.getPath(), request.pathSegments()); | ||
| String query = buildQueryString(request.queryParams()); | ||
| URI resolved = new URI(baseUrl.getProtocol(), baseUrl.getUserInfo(), baseUrl.getHost(), baseUrl.getPort(), | ||
| path, query, null); | ||
| return resolved.toURL(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be internal to the HttpRequest type and we should avoid stitching these pieces together as this could break if something changes in OpenAI library. Instead, can we move this logic to HttpRequest type where it can return a full URL? There should already be a place where this is done in the OpenAI library, we just have to offer that as a public API.
| return null; | ||
| } | ||
| List<HttpPipelinePolicy> orderedPolicies = getOrderedCustomPolicies(); | ||
| if (orderedPolicies.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This builder also supports setting retryPolicy or retryOptions directly (not through addPolicy() method). It looks like that will have no effect on OpenAI HTTP client. Should we consider that to be a custom policy?
|
@jpalvarezl I've opened a new pull request, #47428, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@jpalvarezl I've opened a new pull request, #47429, to work on those changes. Once the pull request is ready, I'll request review from you. |
…mentation/http/OpenAiRequestUrlBuilder.java Co-authored-by: Copilot <[email protected]>
…mentation/http/PolicyDecoratingHttpClient.java Co-authored-by: Copilot <[email protected]>
…pClient (#47429) * Initial plan * Add async and error propagation test coverage for PolicyDecoratingHttpClient Co-authored-by: jpalvarezl <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jpalvarezl <[email protected]> Co-authored-by: Jose Alvarez <[email protected]>
* Initial plan * Add error handling tests for HttpClientHelper Added comprehensive error handling test cases including: - Null request body handling - IOException during body buffering - Malformed URLs - Async execution failures Co-authored-by: jpalvarezl <[email protected]> * Address code review feedback on error handling tests - Simplify FailingHttpRequestBody to directly throw UncheckedIOException - Improve assertion clarity in body buffering test - Remove fragile message checking in malformed URL test Co-authored-by: jpalvarezl <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jpalvarezl <[email protected]> Co-authored-by: Jose Alvarez <[email protected]>
test-proxytool with non-Azure HttpClientscom.azure.ai.agents.ConversationsTests#basicCRUDOperations. The recordings were generated and sanitized correctly in the expected repository here